-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[MLIR] Drop use of REQUIRES:shell from tests #168989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MLIR] Drop use of REQUIRES:shell from tests #168989
Conversation
This patch drops two instances of REQUIRES: shell from MLIR tests. This feature does not mean much given the internal shell is the default for MLIR. It does prevent these tests from running on Windows, but it does not seem like there is anything inherent to these tests preventing them from running on Windows (minus maybe the lack of spirv-tools, which is explicitly required anyways.
|
@llvm/pr-subscribers-mlir-spirv @llvm/pr-subscribers-mlir Author: Aiden Grossman (boomanaiden154) ChangesThis patch drops two instances of REQUIRES: shell from MLIR tests. This feature does not mean much given the internal shell is the default for MLIR. It does prevent these tests from running on Windows, but it does not seem like there is anything inherent to these tests preventing them from running on Windows (minus maybe the lack of spirv-tools, which is explicitly required anyways. Full diff: https://github.com/llvm/llvm-project/pull/168989.diff 2 Files Affected:
diff --git a/mlir/test/Target/SPIRV/mlir-translate.mlir b/mlir/test/Target/SPIRV/mlir-translate.mlir
index cbce351dd35b5..b1966feeb457e 100644
--- a/mlir/test/Target/SPIRV/mlir-translate.mlir
+++ b/mlir/test/Target/SPIRV/mlir-translate.mlir
@@ -1,7 +1,6 @@
// Check that `--spirv-save-validation-files-with-prefix` generates
// a correct number of files.
-// REQUIRES: shell
// RUN: rm -rf %t
// RUN: mkdir %t && mlir-translate --serialize-spirv --no-implicit-module \
// RUN: --split-input-file --spirv-save-validation-files-with-prefix=%t/foo %s \
diff --git a/mlir/test/Target/SPIRV/module.mlir b/mlir/test/Target/SPIRV/module.mlir
index 7e52e549b5266..fb4d9bcfaabd7 100644
--- a/mlir/test/Target/SPIRV/module.mlir
+++ b/mlir/test/Target/SPIRV/module.mlir
@@ -1,6 +1,5 @@
// RUN: mlir-translate --no-implicit-module --test-spirv-roundtrip --split-input-file %s | FileCheck %s
-// REQUIRES: shell
// RUN: %if spirv-tools %{ rm -rf %t %}
// RUN: %if spirv-tools %{ mkdir %t %}
// RUN: %if spirv-tools %{ mlir-translate --no-implicit-module --serialize-spirv --split-input-file --spirv-save-validation-files-with-prefix=%t/module %s %}
|
🐧 Linux x64 Test Results
|
joker-eph
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG if this works without it.
I'm confused by the PR title though, shouldn't it be something like:
[MLIR] Drop REQUIRES:shell not needed from 2 tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, it's an artifact (at least for the module tests) from where we used ls and xargs for validation - we've updated spirv-val since so it handles it for us.
And I agree with above, the title is slightly confusing.
Thanks for cleaning it up!
Updated. Thanks for the suggestion. |
This patch drops two instances of REQUIRES: shell from MLIR tests. This feature does not mean much given the internal shell is the default for MLIR. It does prevent these tests from running on Windows, but it does not seem like there is anything inherent to these tests preventing them from running on Windows (minus maybe the lack of spirv-tools, which is explicitly required anyways.
This patch drops two instances of REQUIRES: shell from MLIR tests. This feature does not mean much given the internal shell is the default for MLIR. It does prevent these tests from running on Windows, but it does not seem like there is anything inherent to these tests preventing them from running on Windows (minus maybe the lack of spirv-tools, which is explicitly required anyways.
This patch drops two instances of REQUIRES: shell from MLIR tests. This feature does not mean much given the internal shell is the default for MLIR. It does prevent these tests from running on Windows, but it does not seem like there is anything inherent to these tests preventing them from running on Windows (minus maybe the lack of spirv-tools, which is explicitly required anyways.